Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WSGI request/response hooks #424

Merged
merged 2 commits into from
Apr 14, 2021
Merged

WSGI request/response hooks #424

merged 2 commits into from
Apr 14, 2021

Conversation

owais
Copy link
Contributor

@owais owais commented Apr 8, 2021 •
edited
Loading

Description

Replaced WSGI name callback with request/response hooks

Fixes #409

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Added test case

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@owais owais requested review from a team, aabmass and srikanthccv and removed request for a team April 8, 2021 04:13
@owais owais force-pushed the wsgi-hooks branch 2 times, most recently from cc6260b to fb7d281 Compare April 8, 2021 04:15
@owais owais added the WIP label Apr 8, 2021
@owais owais changed the title WSGI request/response hooks WIP: WSGI request/response hooks Apr 8, 2021
@owais owais changed the title WIP: WSGI request/response hooks WSGI request/response hooks Apr 8, 2021
@owais owais removed the WIP label Apr 8, 2021
srikanthccv
srikanthccv approved these changes Apr 9, 2021
View reviewed changes
Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I think one downside here is when a user has custom sampler which uses span name as a part of sampling decision then the default span name might not be really helpful. And any update_name calls can't change decision.

Updates the Span name. Upon this update, any sampling behavior based on Span name will depend on the implementation.

Note that Samplers can only consider information already present during span creation. Any changes done later, including updated span name, cannot change their decisions.

@lzchen
Copy link
Contributor

lzchen commented Apr 9, 2021

custom sampler which uses span name as a part of sampling decision then the default span name might not be really helpful

I don't see a workaround for this? If this is needed in the future, name_callback might be needed.

@owais owais requested a review from srikanthccv April 13, 2021 06:10
@lzchen lzchen merged commit 96b0f59 into open-telemetry:main Apr 14, 2021
@owais owais deleted the wsgi-hooks branch April 14, 2021 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lzchen lzchen lzchen approved these changes

@srikanthccv srikanthccv srikanthccv approved these changes

@aabmass aabmass Awaiting requested review from aabmass

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[WSGI] Replace span name callback with request and response hooks
3 participants
@owais @lzchen @srikanthccv